Skip to content

Conversation

@subhashkhileri
Copy link
Member

@subhashkhileri subhashkhileri commented Jan 20, 2026

Description

https://issues.redhat.com/browse/RHIDP-11483

Which issue(s) does this PR fix

  • Fixes #?

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

@christoph-jerolimov
Copy link
Member

/cc @jrichter1 @christoph-jerolimov

@christoph-jerolimov
Copy link
Member

@github-actions
Copy link
Contributor

The image is available at:

/test e2e-ocp-helm

@subhashkhileri subhashkhileri changed the title local test runner chore(e2e) : Add Local Test Runner support for E2E Tests for RHDH core. Jan 20, 2026
@github-actions
Copy link
Contributor

The image is available at:

/test e2e-ocp-helm

@github-actions
Copy link
Contributor

The image is available at:

/test e2e-ocp-helm

@gustavolira
Copy link
Member

@subhashkhileri subhashkhileri changed the title chore(e2e) : Add Local Test Runner support for E2E Tests for RHDH core. chore(e2e) : add Local Test Runner support for E2E Tests for RHDH core. Jan 21, 2026
@subhashkhileri subhashkhileri changed the title chore(e2e) : add Local Test Runner support for E2E Tests for RHDH core. chore : add Local Test Runner support for E2E Tests for RHDH core. Jan 21, 2026
@subhashkhileri subhashkhileri changed the title chore : add Local Test Runner support for E2E Tests for RHDH core. chore: add Local Test Runner support for E2E Tests for RHDH core. Jan 21, 2026
@github-actions
Copy link
Contributor

The image is available at:

/test e2e-ocp-helm

@zdrapela
Copy link
Member

/review
-i

@rhdh-qodo-merge
Copy link

rhdh-qodo-merge bot commented Jan 21, 2026

PR Reviewer Guide 🔍

(Review updated until commit 82835ea)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

RHIDP-11483 - Partially compliant

Compliant requirements:

  • Provide an interactive CLI to select job type, image repository, image tag, and run mode
  • Persist configuration between runs for quick iteration
  • Verify image existence on quay.io before deployment begins
  • Fetch secrets from Vault securely and do not store them on disk
  • Handle Vault authentication, secret management, and cluster access automatically
  • Early crash detection: fail fast if pods enter CrashLoopBackOff
  • Documentation covers usage and examples (at least for OCP + mentions for other clusters)
  • Prereqs are documented and partially enforced via script checks

Non-compliant requirements:

  • Enable developers to run locally against any Kubernetes cluster (OCP, AKS, EKS, GKE) with automatic handling (implementation shown appears OpenShift/oc-specific)
  • Container drops into an interactive shell on failure for debugging

Requires further human verification:

  • Containerized test execution mirrors CI environment (needs validation of image parity and runtime behavior)
  • Deploy only and then run tests locally in headed mode for debugging (needs end-to-end validation on a real cluster)
  • Documentation correctness for “all supported clusters” and any required cluster-specific steps (AKS/EKS/GKE paths)
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🔒 Security concerns

Privileged credentials / token exposure risk:
e2e-tests/local-run.sh grants cluster-admin to a newly-created service account and generates a 48h token (then passes OC_TOKEN / K8S_CLUSTER_TOKEN into the container via environment variables). Environment variables can be exposed through process inspection, crash dumps, or verbose logging, and the role is maximum privilege. Consider reducing RBAC to minimum required, shortening token duration, and adding explicit cleanup instructions/automation (namespace/serviceaccount/rolebinding removal) after runs.
Sensitive token propagation: Vault authentication is performed on the host and a Vault token is exported/passed to the container (VAULT_TOKEN). Ensure logs never echo this value, and consider using Vault agent / short-lived child tokens with limited policies instead of forwarding a broad token.

⚡ Recommended focus areas for review

Security

The script creates a service account and grants it cluster-admin, then mints a long-lived token and passes it into the container via environment variables. This is a very high-privilege credential flow; reviewer should validate least-privilege RBAC is not feasible, token scope/cleanup behavior, and that logs/artifacts never print or persist these credentials.

log::section "Setting up cluster access"
SA_NAME="rhdh-local-tester"
SA_NAMESPACE="rhdh-local-test"
OC_SERVER=$(oc whoami --show-server)
oc create namespace "$SA_NAMESPACE" 2>/dev/null || log::info "Namespace already exists"
oc create serviceaccount "$SA_NAME" -n "$SA_NAMESPACE" 2>/dev/null || log::info "Service account already exists"
oc adm policy add-cluster-role-to-user cluster-admin "system:serviceaccount:${SA_NAMESPACE}:${SA_NAME}" 2>/dev/null || true
OC_TOKEN=$(oc create token "$SA_NAME" -n "$SA_NAMESPACE" --duration=48h)
Secret handling

Vault secrets are written as files under /tmp/secrets/. Although the container mounts /tmp/secrets as tmpfs, reviewer should validate nothing else copies these files into artifacts/workdir, and that filenames derived from Vault keys cannot cause unexpected path traversal or collisions.

# Fetch and write secrets to /tmp/secrets/
log::section "Fetching Vault Secrets"
SECRETS=$(vault kv get -format=json -mount="kv" "selfservice/rhdh-qe/rhdh" | jq -r ".data.data")

for key in $(echo "$SECRETS" | jq -r "keys[]"); do
    if [[ "$key" == */* ]]; then
        mkdir -p "/tmp/secrets/$(dirname "$key")"
    fi
    echo "$SECRETS" | jq -r --arg k "$key" '.[$k]' > "/tmp/secrets/$key"
done
📚 Focus areas based on broader codebase context

Portability

container-init.sh hardcodes the log library path (/tmp/rhdh/.ibm/pipelines/lib/log.sh) and sources it without validating the repo location, which makes the script fragile if the mount point or working directory changes. Consider deriving the repo root dynamically (e.g., via script directory or an env var) and failing with a clear error if the file is missing. (Ref 3, Ref 4)

# shellcheck source=../.ibm/pipelines/lib/log.sh
source "/tmp/rhdh/.ibm/pipelines/lib/log.sh"

Reference reasoning: Existing bash utilities consistently compute paths from variables and guard directory changes/critical steps with || return 1 to avoid assuming a fixed filesystem layout. Following the same approach here would align with established patterns and reduce failure modes when running scripts from different locations.

📄 References
  1. redhat-developer/rhdh/scripts/update-Dockerfile.sh [1-23]
  2. redhat-developer/rhdh/scripts/patch-package.sh [42-67]
  3. redhat-developer/rhdh/scripts/patch-package.sh [112-134]
  4. redhat-developer/rhdh/scripts/patch-package.sh [1-40]
  5. redhat-developer/rhdh/scripts/patch-package.sh [100-104]
  6. redhat-developer/rhdh/scripts/rhdh-openshift-setup/quick-start-rhdh.sh [1-7]
  7. redhat-developer/rhdh/scripts/rhdh-openshift-setup/quick-start-rhdh.sh [27-43]
  8. redhat-developer/rhdh/scripts/rhdh-openshift-setup/quick-start-rhdh.sh [199-244]

Instead of hardcoding nodejs:18-ubi8, query the cluster for available
nodejs imagestream tags and select the latest UBI9 version. Falls back
to 18-ubi8 if no UBI9 tags are found.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@github-actions
Copy link
Contributor

🚫 Image Push Skipped.

The container image push was skipped because the build was skipped (either due to [skip-build] tag or no relevant changes with existing image)

Copy link
Member

@zdrapela zdrapela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinions here, but I have some thoughts on this 🤔

But great work overall!

Comment on lines +121 to +124
2) JOB_NAME="periodic-ci-ocp-helm-nightly" ;;
3) JOB_NAME="periodic-ci-ocp-operator-nightly" ;;
4) JOB_NAME="periodic-ci-ocp-helm-upgrade-nightly" ;;
5) JOB_NAME="periodic-ci-ocp-operator-auth-providers-nightly" ;;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It won't have an effect, but we can align the job names with what's really used.

Suggested change
2) JOB_NAME="periodic-ci-ocp-helm-nightly" ;;
3) JOB_NAME="periodic-ci-ocp-operator-nightly" ;;
4) JOB_NAME="periodic-ci-ocp-helm-upgrade-nightly" ;;
5) JOB_NAME="periodic-ci-ocp-operator-auth-providers-nightly" ;;
2) JOB_NAME="periodic-ci-redhat-developer-rhdh-main-ocp-helm-nightly" ;;
3) JOB_NAME="periodic-ci-redhat-developer-rhdh-main-ocp-operator-nightly" ;;
4) JOB_NAME="periodic-ci-redhat-developer-rhdh-main-ocp-helm-upgrade-nightly" ;;
5) JOB_NAME="periodic-ci-redhat-developer-rhdh-main-ocp-operator-auth-providers-nightly" ;;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its release / branch independent. the main might confuse.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, we can keep it as it is

@@ -0,0 +1,335 @@
#!/bin/bash
Copy link
Member

@zdrapela zdrapela Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I prefer to have the option to use flags to configure the options that I want. The current setup with a walkthrough works fine, and it's great for a newcomer, etc. The option to rerun the same config is great. But for me, flags are unbeatable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flags now work great!

@@ -0,0 +1,141 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently have linters for bash scripts only in .ibm folder, and basically all the bash scripts related to e2e and CI are there. It can make sense to have those scripts in e2e-tests folder, but it's kind of mixing up TypeScript with Bash now 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are user-facing scripts that belong with the e2e-tests, not CI pipeline

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but it would make sense to at least group them in a folder, maybe? So we can set up Prettier and ShellCheck (possibly in the future)

Co-authored-by: Zbyněk Drápela <61500440+zdrapela@users.noreply.github.com>
@github-actions
Copy link
Contributor

🚫 Image Push Skipped.

The container image push was skipped because the build was skipped (either due to [skip-build] tag or no relevant changes with existing image)

@github-actions
Copy link
Contributor

🚫 Image Push Skipped.

The container image push was skipped because the build was skipped (either due to [skip-build] tag or no relevant changes with existing image)

Add CLI flags (-j, -r, -t, -p, -s) to local-run.sh for automation and
quick runs without interactive prompts. Also improve container-init.sh
to pre-compute and save config before deployment so URLs are available
even if deployment fails.

Additional fixes:
- Fix secrets parsing in local-test-setup.sh to handle special chars
- Minor formatting fix in utils.sh for piped command

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@github-actions
Copy link
Contributor

@openshift-ci
Copy link

openshift-ci bot commented Jan 22, 2026

@subhashkhileri: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ocp-helm 82835ea link true /test e2e-ocp-helm

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@zdrapela
Copy link
Member

/review
-i

@rhdh-qodo-merge
Copy link

Persistent review updated to latest commit 82835ea

Copy link
Member

@zdrapela zdrapela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say this is ready to be used by RHDH devs. It works as expected with OCP

@openshift-ci
Copy link

openshift-ci bot commented Jan 23, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zdrapela
Once this PR has been reviewed and has the lgtm label, please assign 04kash for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

log::info "To run tests:"
echo " cd e2e-tests"
echo " yarn install"
echo " yarn playwright test --headed"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this can contain the --project=... with the specific project. Without the project specified, it will run all of them. Also the --headed can be explained (you often want to run it headlessly)

Extend local test runner to support non-OpenShift Kubernetes platforms:
- Derive CONTAINER_PLATFORM from job name (*aks*, *eks*, *gke*, *osd*)
- Add platform-specific cluster login (oc for OCP/OSD, kubectl for others)
- Handle service account token creation for non-OpenShift clusters
- Update container-init.sh to configure kubectl context for AKS/EKS/GKE
- Add interactive menu options for AKS, EKS, GKE job selection
- Update help text and examples for multi-platform usage

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@github-actions
Copy link
Contributor

🚫 Image Push Skipped.

The container image push was skipped because the build was skipped (either due to [skip-build] tag or no relevant changes with existing image)

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants